-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block with statistics by lessons quantity #2616
Block with statistics by lessons quantity #2616
Conversation
Renatavl
commented
Oct 14, 2024
import QuantityLessonsChart from './QuantityLessonsChart' | ||
import { useTranslation } from 'react-i18next' | ||
|
||
const currencies = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relocate to .constants.tsx
|
||
const currencies = [ | ||
{ | ||
value: 'USD', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create enum for currencies
} | ||
] | ||
|
||
const subjects = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subjects should be taken from user mainSubjects
} | ||
] | ||
|
||
const years = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use Date here
backgroundColor: '#79B260', | ||
borderColor: '#79B260', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use palette
datasets: [ | ||
{ | ||
label: 'Lessons', | ||
data: [5, 5, 10, 10, 12, 15, 14, 14, 10, 10, 12, 12], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this data represent accepted offers?
display: false | ||
}, | ||
ticks: { | ||
color: '#455A64', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use palette
] | ||
} | ||
|
||
const options = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
relocate to .constants.tsx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This object directly belongs to component properties, but not a hardcoded value which is used as input data for the component. That's why it is better to leave it at the same place as the component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand why we can't move these constant values to constants.tsx file, because we store constants in separate files to avoid cluttering files with components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can move these constants to another file and just import them, because now the component is heavily cluttered
|
||
const data = { | ||
labels: [ | ||
'JAN', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its probably better to use Date here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not needed because in the end result we will get the same results as with Date. We simply have a fixed list of months that doesn't change in any way and doesn't depend on anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is the QuantityLessonsChart component located in a folder meant for another component?
] | ||
} | ||
|
||
const options = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't understand why we can't move these constant values to constants.tsx file, because we store constants in separate files to avoid cluttering files with components
e8c10fd
to
28db1e0
Compare
{ | ||
value: 'Music' | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the blank lines
color: 'basic.darkGray', | ||
mb: '5px' | ||
}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with blank lines
28db1e0
to
dd0816e
Compare
Quality Gate passedIssues Measures |